Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 Add branch protection evaluation #3759

Merged
merged 25 commits into from
Feb 28, 2024
Merged

🌱 Add branch protection evaluation #3759

merged 25 commits into from
Feb 28, 2024

Conversation

AdamKorcz
Copy link
Contributor

What kind of change does this PR introduce?

(Is it a bug fix, feature, docs update, something else?)

What is the current behavior?

What is the new behavior (if this is a feature change)?**

  • Changes Branch Protection evaluation to process probe findings. Some of the probes were submitted here: 🌱 Add probes for Branch Protection #3691

  • Adds another probe

  • Rewrites a bunch of tests

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

NONE

Special notes for your reviewer

Does this PR introduce a user-facing change?

NONE
For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

@AdamKorcz AdamKorcz requested a review from a team as a code owner December 29, 2023 19:33
@AdamKorcz AdamKorcz requested review from spencerschrock and laurentsimon and removed request for a team December 29, 2023 19:33
@codecov Codecov
Copy link

codecov bot commented Dec 29, 2023 •
edited
Loading

Codecov Report

Merging #3759 (f706b69) into main (4ae4ba2) will decrease coverage by 6.76%.
The diff coverage is 76.53%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3759      +/-   ##
==========================================
- Coverage   75.36%   68.60%   -6.76%     
==========================================
  Files         232      234       +2     
  Lines       15682    15842     +160     
==========================================
- Hits        11818    10868     -950     
- Misses       3129     4302    +1173     
+ Partials      735      672      -63

spencerschrock
spencerschrock reviewed Jan 2, 2024
View reviewed changes
Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only a partial review (ran out of time before lunch), but wanted to get a few of these questions to you for discussion. Particularly the "protected" branch value/probe discussion

checks/evaluation/branch_protection.go Outdated Show resolved Hide resolved
checks/evaluation/branch_protection.go Outdated Show resolved Hide resolved
checks/evaluation/branch_protection.go Show resolved Hide resolved
checks/evaluation/branch_protection.go Outdated Show resolved Hide resolved
checks/evaluation/branch_protection.go Outdated Show resolved Hide resolved
probes/blocksDeleteOnBranches/impl.go Outdated Show resolved Hide resolved
probes/requiresCodeOwnersReview/impl.go Outdated Show resolved Hide resolved
@AdamKorcz AdamKorcz temporarily deployed to integration-test January 8, 2024 20:17 — with GitHub Actions Inactive
@AdamKorcz
Copy link
Contributor Author

#3759 (comment) fixed in ba40d03

@AdamKorcz
Copy link
Contributor Author

The failing integration test looks unrelated, but reliably reproduces:

• [FAILED] [0.487 seconds]
E2E TEST:SearchCommits E2E TEST:SearchCommits [It] Should return commits by dependabot
/home/runner/work/scorecard/scorecard/e2e/searchCommits_test.go:29

  [FAILED] Expected
      <int>: 0
  to be >
      <int>: 0
  In [It] at: /home/runner/work/scorecard/scorecard/e2e/searchCommits_test.go:37 @ 01/09/24 17:24:45.055

@spencerschrock
Copy link
Member

The failing integration test looks unrelated, but reliably reproduces:

I think this had to do with today's GitHub status incident.

@AdamKorcz
Bring back negative outcome in case of 0 codeowners files
75c18eb 
Signed-off-by: Adam Korczynski <[email protected]>
@AdamKorcz
log based on whether branches are protected
48c1b53 
Signed-off-by: Adam Korczynski <[email protected]>
@AdamKorcz
remove unnecessary test
2d26086 
Signed-off-by: Adam Korczynski <[email protected]>
@AdamKorcz
debug failing tests
00acf66 
Signed-off-by: Adam Korczynski <[email protected]>
@AdamKorcz
Fix failing tests
175d28f 
Signed-off-by: Adam Korczynski <[email protected]>
@AdamKorcz
rename test
aeb5a55 
Signed-off-by: Adam Korczynski <[email protected]>
@AdamKorcz
update to with latest upstream changes
e5b1666 
Signed-off-by: AdamKorcz <[email protected]>
@AdamKorcz
fix linting issues
21fdf63 
Signed-off-by: AdamKorcz <[email protected]>
@AdamKorcz
remove tests that represent impossible scenarios
a4896eb 
Signed-off-by: AdamKorcz <[email protected]>
This was referenced Feb 26, 2024
Closed
Closed
@spencerschrock
Copy link
Member

spencerschrock commented Feb 26, 2024 •
edited
Loading

@spencerschrock PTAL again.

I'm seeing weird reversions of commits from earlier discussions. For example:

And one e2e test is still failing

TOKEN_TYPE=PAT ginkgo --focus-file branch_protection_test.go e2e

[FAILED] branch protection accessible patch: (-expected +actual)  utests.TestReturn{
        Error:         nil,
        Score:         1,
  -     NumberOfWarn:  3,
  +     NumberOfWarn:  6,
        NumberOfInfo:  4,
  -     NumberOfDebug: 4,
  +     NumberOfDebug: 8,
    }

@spencerschrock
remove protected finding value
93fbbc1 
This was discussed previously, but accidentally reverted

Signed-off-by: Spencer Schrock <[email protected]>
@spencerschrock
Revert "debug failing tests"
6280031 
This reverts commit 00acf66.

Signed-off-by: Spencer Schrock <[email protected]>
@spencerschrock
use branchName key for branch name
b51e620 
Signed-off-by: Spencer Schrock <[email protected]>
@spencerschrock
include number of reviews in INFO
8358285 
this was previously included by the old evaluation code

Signed-off-by: Spencer Schrock <[email protected]>
@spencerschrock
reduce info count by 1
6123cef 
requiring codeowners without a corresponding file used to give 1 INFO and 1 WARN
now it only gives 1 WARN

Signed-off-by: Spencer Schrock <[email protected]>
@spencerschrock
Merge branch 'main' into pr/AdamKorcz/3759
f706b69 
Signed-off-by: Spencer Schrock <[email protected]>
@spencerschrock spencerschrock mentioned this pull request Feb 27, 2024
Closed
@spencerschrock
Copy link
Member

/scdiff generate Branch-Protection

@github-actions GitHub Actions
Copy link

Here's a link to the scdiff run

raghavkaul
raghavkaul approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@raghavkaul raghavkaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

93fbbc1-f706b69 LGTM.

@spencerschrock spencerschrock merged commit 4daefb6 into ossf:main Feb 28, 2024
38 checks passed
@spencerschrock spencerschrock mentioned this pull request Feb 29, 2024
Merged
1 task
fhoeborn pushed a commit to fhoeborn/scorecard that referenced this pull request Apr 1, 2024
@AdamKorcz @spencerschrock @fhoeborn
🌱 Add branch protection probe evaluation (ossf#3759)
c810d37 
* 🌱 Add branch protection evaluation

Signed-off-by: Adam Korczynski <[email protected]>

* make helper for getting the branchName

Signed-off-by: Adam Korczynski <[email protected]>

* move check for branch name

Signed-off-by: Adam Korczynski <[email protected]>

* define size of slice

Signed-off-by: Adam Korczynski <[email protected]>

* add probe for protected branches.

Signed-off-by: Adam Korczynski <[email protected]>

* change 'basicNonAdminProtection' to 'deleteAndForcePushProtection'

Signed-off-by: Adam Korczynski <[email protected]>

* fix markdown in text field in def.yml

Signed-off-by: Adam Korczynski <[email protected]>

* remove duplicate conditional

Signed-off-by: Adam Korczynski <[email protected]>

* remove redundant 'protected' value from 'requiresCodeOwnersReview' probe

Signed-off-by: Adam Korczynski <[email protected]>

* remove protected values from probes

Signed-off-by: Adam Korczynski <[email protected]>

* Bring back negative outcome in case of 0 codeowners files

Signed-off-by: Adam Korczynski <[email protected]>

* log based on whether branches are protected

Signed-off-by: Adam Korczynski <[email protected]>

* remove unnecessary test

Signed-off-by: Adam Korczynski <[email protected]>

* debug failing tests

Signed-off-by: Adam Korczynski <[email protected]>

* Fix failing tests

Signed-off-by: Adam Korczynski <[email protected]>

* rename test

Signed-off-by: Adam Korczynski <[email protected]>

* update to with latest upstream changes

Signed-off-by: AdamKorcz <[email protected]>

* fix linting issues

Signed-off-by: AdamKorcz <[email protected]>

* remove tests that represent impossible scenarios

Signed-off-by: AdamKorcz <[email protected]>

* remove protected finding value

This was discussed previously, but accidentally reverted

Signed-off-by: Spencer Schrock <[email protected]>

* Revert "debug failing tests"

This reverts commit 00acf66.

Signed-off-by: Spencer Schrock <[email protected]>

* use branchName key for branch name

Signed-off-by: Spencer Schrock <[email protected]>

* include number of reviews in INFO

this was previously included by the old evaluation code

Signed-off-by: Spencer Schrock <[email protected]>

* reduce info count by 1

requiring codeowners without a corresponding file used to give 1 INFO and 1 WARN
now it only gives 1 WARN

Signed-off-by: Spencer Schrock <[email protected]>

---------

Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: Spencer Schrock <[email protected]>
Co-authored-by: Spencer Schrock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diogoteles08 diogoteles08 diogoteles08 left review comments

@spencerschrock spencerschrock spencerschrock approved these changes

@raghavkaul raghavkaul raghavkaul approved these changes

@laurentsimon laurentsimon Awaiting requested review from laurentsimon laurentsimon is a code owner automatically assigned from ossf/scorecard-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AdamKorcz @spencerschrock @raghavkaul @diogoteles08